Conversation
Reviewer's GuideThis PR centralizes Docker image pulling logic, adds utilities for inspecting and retrieving image labels, introduces generic map helpers for tests, defines required image labels, and extends acceptance tests to validate image metadata. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @petrpinkas - I've reviewed your changes - here's some feedback:
- You’re creating a new Docker client (and deferring Close) in almost every helper – consider centralizing client creation (and context passing) so you can reuse a single client and avoid repeated setup/teardown.
- In FileFromImage you
panicon client initialization failure – it would be better to return an error instead so the caller can handle failures gracefully rather than crashing the test binary. - Most functions call context.TODO(); accepting a context parameter from the caller would make it easier to manage timeouts and cancellations across these image operations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re creating a new Docker client (and deferring Close) in almost every helper – consider centralizing client creation (and context passing) so you can reuse a single client and avoid repeated setup/teardown.
- In FileFromImage you `panic` on client initialization failure – it would be better to return an error instead so the caller can handle failures gracefully rather than crashing the test binary.
- Most functions call context.TODO(); accepting a context parameter from the caller would make it easier to manage timeouts and cancellations across these image operations.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
This is how labels report looks like: |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider refactoring Docker client creation (and closing) into a shared helper so you don’t recreate a new client in every utility function.
- In FileFromImage you’re panicking on client init error—switch that to returning an error to keep error handling consistent across your utilities.
- Rather than using context.TODO inside InspectImageForLabels and PullImageIfNotPresentLocally, accept a context.Context parameter to enable cancellation and timeouts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring Docker client creation (and closing) into a shared helper so you don’t recreate a new client in every utility function.
- In FileFromImage you’re panicking on client init error—switch that to returning an error to keep error handling consistent across your utilities.
- Rather than using context.TODO inside InspectImageForLabels and PullImageIfNotPresentLocally, accept a context.Context parameter to enable cancellation and timeouts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kdacosta0
approved these changes
Aug 28, 2025
Member
kdacosta0
left a comment
kdacosta0
left a comment
There was a problem hiding this comment.
Great PR
I've left two minor comments in the code: one required typo fix and one non-blocking suggestion.
I tested this locally: the tests pass against the 1.3.0 snapshot and correctly fail when I introduce an invalid expectation - the logic works perfectly
LGTM, approved once the typo is addressed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check labels of all images
Currently we check only few basic labels, other will be decided later.
Other improvement is, that images are pulled only once for all the tests. This improves performance significantly.
Summary by Sourcery
Add utilities and tests for inspecting and validating Docker image labels, refactor existing image handling to reuse a shared pull function, and extend test support with generic map operations
New Features:
Enhancements:
Tests:
Summary by Sourcery
Add reusable utilities for pulling Docker images and inspecting their labels, refactor existing image handling to use the new pull logic, and extend acceptance tests to validate image labels using generic map helpers
New Features:
Enhancements:
Tests: